-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Normalizing OG errors #1600
Normalizing OG errors #1600
Conversation
WalkthroughThe changes primarily focus on improving error handling, logging, and response writing across the codebase. New functions have been introduced to handle Ethereum-related and general errors. Unused imports and constants have been removed, and test cases have been updated. The changes also include modifications to handle disconnects and nil responses more effectively. Changes
TipsChat with CodeRabbit Bot (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 1
Files selected for processing (8)
- integration/obscurogateway/obscurogateway_test.go (6 hunks)
- tools/walletextension/accountmanager/account_manager.go (1 hunks)
- tools/walletextension/api/routes.go (5 hunks)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/common/constants.go (2 hunks)
- tools/walletextension/common/json.go (1 hunks)
- tools/walletextension/userconn/user_conn.go (6 hunks)
- tools/walletextension/wallet_extension.go (1 hunks)
Files skipped from review due to trivial changes (4)
- tools/walletextension/api/routes.go
- tools/walletextension/common/constants.go
- tools/walletextension/userconn/user_conn.go
- tools/walletextension/wallet_extension.go
Additional comments (Suppressed): 12
tools/walletextension/common/json.go (1)
- 1-20: The new
JSONRPCMessage
andJSONError
types are well-structured and align with the JSON-RPC specification. The use ofjson.RawMessage
forID
,Params
, andResult
allows for flexibility in handling different data types. TheJSONError
type also aligns with the JSON-RPC error object specification, withCode
,Message
, and optionalData
fields. This should improve error handling consistency across the codebase.tools/walletextension/accountmanager/account_manager.go (1)
- 312-320: The error handling mechanism has been updated to directly write the error message to the user connection instead of using the
HandleError
method. This change aligns with the PR's goal of normalizing error handling. However, the comment on line 315 indicates that there is still work to be done to properly handle disconnections on the user side. This should be addressed to ensure robust error handling.- userConn.HandleError(err.Error()) + err = userConn.WriteResponse([]byte(err.Error())) + if err != nil { + m.logger.Error("unable to close connection with the user", log.ErrKey, err) + }tools/walletextension/api/utils.go (3)
83-85: The function
getUserID
has been left unchanged. It retrieves the user ID from either query parameters or the URL path.87-119: The new function
handleEthError
is introduced to handle Ethereum related errors. It creates a JSON-RPC error message and writes it back to the client. The function takes in anRPCRequest
, aUserConn
, a logger, and an error. It constructs aJSONRPCMessage
with the error details and sends it back to the client. The error handling and logging are done correctly.121-125: The function
handleError
is introduced to handle general errors. It writes the error message back to the client. The function takes in aUserConn
, a logger, and an error. The error handling and logging are done correctly.integration/obscurogateway/obscurogateway_test.go (7)
3-24: The import statements have been updated to include new packages for error handling and JSON-RPC messages. Ensure that these packages are correctly installed and available in the project environment.
50-55: The
createObscuroNetwork
function no longer returns a value. Ensure that this change does not affect other parts of the code that might be expecting a return value from this function.81-97: Two new test functions,
testAreTxsMinted
andtestErrorHandling
, have been added to the test suite. Ensure that these tests are correctly implemented and that they cover all the necessary scenarios.99-110: The wallet creation process has been updated to use
wallet.NewInMemoryWalletFromConfig
instead ofwallets.L2FaucetWallet
. Ensure that this change does not affect the functionality of the tests.122-125: The transaction receipt status is now being checked directly in the test function. This is a good practice as it allows for immediate feedback if the transaction fails.
127-168: The
testErrorHandling
function has been added to test the new error handling mechanism. It makes several requests with different error scenarios and checks the response format. This is a good practice as it ensures that the error handling mechanism works as expected.222-228: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [222-245]
The
createObscuroNetwork
function no longer returns a value. This change simplifies the function and makes it easier to understand. However, ensure that this change does not affect other parts of the code that might be expecting a return value from this function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- tools/walletextension/api/utils.go (2 hunks)
Additional comments (Suppressed): 3
tools/walletextension/api/utils.go (3)
3-13: The new hunk introduces two new imports:
github.com/obscuronet/go-obscuro/go/common/log
andgithub.com/ethereum/go-ethereum/log
. Ensure that these packages are used appropriately in the code and that they do not conflict with each other.88-120: The
handleEthError
function is introduced to handle Ethereum-related errors. It creates aJSONRPCMessage
with the error details and writes it back to the user. The function handles the case where the request isnil
and sets default values formethod
andid
. It also handles any errors that occur during marshalling theJSONRPCMessage
and writing the response back to the user. This is a good practice as it ensures that the user is always informed about what went wrong.122-126: The
handleError
function is introduced to handle generic errors. It writes the error message back to the user and handles any errors that occur during this process. This is a good practice as it ensures that the user is always informed about what went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Status
Actionable comments generated: 2
Files selected for processing (3)
- tools/walletextension/api/utils.go (2 hunks)
- tools/walletextension/test/wallet_extension_test.go (2 hunks)
- tools/walletextension/wallet_extension.go (2 hunks)
Files skipped from review due to trivial changes (2)
- tools/walletextension/test/wallet_extension_test.go
- tools/walletextension/wallet_extension.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Why this change is needed
Part of https://github.com/obscuronet/obscuro-internal/issues/2275
The ethereum error structure is :
{"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"invalid argument 0: json: cannot unmarshal hex string of odd length into Go value of type common.Address"}}
The gateway error structure is:
{"error":{"message":"unable to create VK encryptor - unable to create vk encryption for request - invalid viewing key signature for requested address"}}
Changing the error struct to match the jrpc error structure
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks